-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option printDirective to printSchema #3362
Conversation
Resolves graphql#2020 I initially thought about implementing this as graphql#2020 (comment), but really could not think of a good use case where a directive should be printed in a modified way. Thus, I went for a simpler signature: ```ts shouldPrintDirective?: (directiveNode: DirectiveNode) => boolean ``` This is only a partial implementation for now, there are more possible locations in a schema that can have a directive. Before I finish the implementation, I would like to validate the direction is right.
@spawnia Thanks for opening draft PR. Example: export const GraphQLDeprecatedDirective: GraphQLDirective =
new GraphQLDirective({
name: 'deprecated',
locations: [
DirectiveLocation.FIELD_DEFINITION,
// ...
],
args: {
reason: {
type: GraphQLString,
defaultValue: DEFAULT_DEPRECATION_REASON,
},
},
notSureWhatNameToChoose(obj) {
if (obj.deprecationReason != null) {
return [
obj.deprecationReason === DEFAULT_DEPRECATION_REASON
? {}
: { reason: obj.deprecationReason },
];
}
}
}); That way directives can leave in separate packages and be fully self-contained. |
Hey @IvanGoncharov, thanks for the feedback on the PR. It does surface a very interesting point, that is how we can bridge the gap for features such as Representation
Conversion
With those built-in features that are also available in introspection, I do not see a use case why they should ever not be present in a printed schema. They are already exposed through introspection anyways, and thus not expected to contain internal-only information. Thus, I think we could simply always print them. If there are concerns regarding backwards compatiblity, we could add a config setting such as
Your example makes it quite clear why it would be useful for directives to influence how they are printed. Howerver, in a purely SDL driven implementation, not every used directive necessarily even has a code based definition associated with it. The main project I am building (https://github.com/nuwave/lighthouse) only defines directives through SDL, which has been sufficient for validation and all other purposes. We would require a default implementation of this method anyways - probably just returning all the arguments? I would argue that this functionality is related but not essential to the problem I am trying to solve here.
If it must exclusively work with
We should offer a generalized implementation of this that smooths the path from SDL -> Code -> SDL for implementations that are primarily SDL-driven. |
@spawnia Not sure I understand your arguments, yes built-in stuff should be printable.
|
You are right, I completely missed that. Since |
| GraphQLEnumValue | ||
| GraphQLInputField | ||
| GraphQLArgument, | ||
) => ReadonlyArray<ConstDirectiveNode>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there perhaps a better data structure to return? I don't want to go with string
, as that puts the burden of proper formatting on the end users.
|
||
import { astFromValue } from './astFromValue'; | ||
|
||
export function printSchema(schema: GraphQLSchema): string { | ||
export interface PrintSchemaOptions { | ||
printDirectives?: ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps splitting this into separate monomorphic functions would make it easier for the end user and avoid them having to do type checks:
printDirectivesForSchema: (schema: GraphQLSchema): ReadonlyArray<ConstDirectiveNode>;
printDirectivesForType: (type: GraphQLType): ReadonlyArray<ConstDirectiveNode>;
...
@spawnia It should be tied to directive objects, otherwise it's not modular. |
That implies we tie directive objects to definitions in a unified manner. That seems to contradict the philosophy of not letting directives bleed into the programmatic representation of a schema. The implementation I propose is agnostic as to if or how directives are tied into the schema. It can leverage AST nodes if the schema came from an SDL source, it can use something the developer placed in
I am not convinced that schema-agnostic and universally uniform printing is needed. It may even be necessary to do custom printing, for instance in Apollo federation: https://www.apollographql.com/docs/federation/federation-spec/#fetch-service-capabilities |
@spawnia It's exactly a problem we have right now.
If we truly want to solve this problem, the schema should be self-printable.
Directive definitions are already part of the schema even SDL directives (you can check introspection).
So "supplying additional metadata" is directly mapped to Back to your proposal, let's use your framework as an example. |
I removed everything that was tied directly to the SDL source. The implementation is now fully agnostic to what will result in the printed directives in the final schema. I think this implementation is useful on its own. |
extracted from graphql#3362
Extracted from graphql#3362 Co-authored-by: spawnia <[email protected]>
Extracted from graphql#3362 Co-authored-by: spawnia <[email protected]>
Closing this for now. Note that graphql-tools has a https://github.com/ardatan/graphql-tools/blob/master/packages/utils/src/print-schema-with-directives.ts tool. The reference implementation has decided not to support it, as technically it's unsafe, we can't safely recreate the original directives from a given schema, because the directives could be theoretically used in any which way by a schema builder, i.e. could modify the schema. My understanding is that the function in See related discussion in terms of introspection and a still potentially in-progress effort there: |
Resolves #2020
I initially thought about implementing this as #2020 (comment),
but really could not think of a good use case where
a directive should be printed in a modified way.
Thus, I went for a simpler signature:
This is only a partial implementation for now, there are more possible
locations in a schema that can have a directive. Before I finish the
implementation, I would like to validate the direction is right.